fix(sandbox): interpolate host-side hint in channels add/remove guard - #7295
Conversation
📝 WalkthroughWalkthroughThe sandboxed ChangesSandbox channel guard
Estimated code review effort: 2 (Simple) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — InformationalAdvisor assessment: Informational / high confidence Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: None 1 optional E2E recommendation
1 warning · 0 suggestionsWarningsWarnings do not block.
|
|
✨ Thanks for the fix. This makes the sandbox guard error actionable by interpolating the actual sandbox name, subcommand, and channel argument. Related open issues: |
The in-sandbox guard for `openclaw channels add/remove` printed a generic hint with literal `<sandbox>` and `<channel>` placeholders, making it impossible for automation (and difficult for users) to determine the correct host-side command. Replace the multi-line generic hint with a single actionable line that interpolates the actual sandbox name via the existing `_nemoclaw_policy_denial_hint_label()` helper, the actual subcommand ($2), and the actual channel argument ($3) when present. Before: nemoclaw <sandbox> channels add <channel> After: Run 'nemoclaw my-assistant channels add telegram' on the host. Closes NVIDIA#7292 Signed-off-by: kagura-agent <kagura.agent.ai@gmail.com>
079369d to
6376ca0
Compare
|
Rebased onto latest main to retrigger CI. The previous |
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
left a comment
There was a problem hiding this comment.
Reviewed exact head 38fb557. The host-side hint now uses exact operation, channel, and sandbox allowlists; hostile-input tests prove raw markers cannot reach the copyable command. Automated review has zero findings, all threads are resolved, ordinary fork CI is green after one unrelated timing-flake rerun, and the E2E gate correctly reports no required live target.
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
ericksoa
left a comment
There was a problem hiding this comment.
Approved at exact head f1609e3 after full same-attempt CI (run 30143726018 attempt 4), required E2E, CodeQL/ShellCheck/advisor checks, exact diff/security review, current documentation receipt, and a zero-unresolved-thread sweep. The copyable host command is constrained by operation/channel allowlists and covered by hostile-input tests. No merge performed.
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Add the canonical `docs/changelog/2026-07-25.mdx` release entry with the exact `## v0.0.96` heading. The entry reconciles all 90 first-parent commits since v0.0.95 with all 92 merged PRs in the live `v0.0.96` label ledger and groups the user-visible changes by operator journey. ## Changes - Add the parser-safe dated MDX changelog entry for v0.0.96 with root-absolute links to the focused user guides. - Source summary: - [#7194](#7194) -> `docs/changelog/2026-07-25.mdx`: Document persistent baseline network policy exclusions and their inspection, rebuild, and snapshot behavior. - [#7188](#7188), [#7427](#7427), and [#7546](#7546) -> `docs/changelog/2026-07-25.mdx`: Document DNS-backed HTTPS inference routing, keyless loopback endpoints, and provider-marker isolation. - [#7238](#7238) -> `docs/changelog/2026-07-25.mdx`: Document blueprint sandbox and provider identifier validation before state writes or OpenShell calls, with bounded terminal-safe rejection previews. - [#7319](#7319), [#7274](#7274), [#7528](#7528), [#7353](#7353), and [#7560](#7560) -> `docs/changelog/2026-07-25.mdx`: Document the managed default gateway service, onboarding readiness, and container-runtime identity safeguards. - [#7349](#7349), [#7498](#7498), [#7406](#7406), [#7196](#7196), [#7559](#7559), [#7421](#7421), [#7510](#7510), [#7295](#7295), and [#7565](#7565) -> `docs/changelog/2026-07-25.mdx`: Document gateway-scoped status, lifecycle diagnostics, managed MCP recovery, delete-edge safeguards, and fail-closed CLI prompt and command output. - [#7591](#7591) -> `docs/changelog/2026-07-25.mdx`: Document opt-in authenticated MCP tool-name discovery, its bounded and names-only contract, probe interaction, and rebuild requirement. - [#7305](#7305), [#7480](#7480), [#7471](#7471), [#7365](#7365), and [#7541](#7541) -> `docs/changelog/2026-07-25.mdx`: Document installer version checks, version-tag reporting, license guidance, WSL Ollama selection, and DGX Station vLLM detection. - [#7482](#7482), [#7466](#7466), [#7208](#7208), [#7434](#7434), and [#7586](#7586) -> `docs/changelog/2026-07-25.mdx`: Document Ollama resource details, reasoning precedence, Hermes onboarding behavior, and preserved managed Hermes BuildKit failures. - [#6830](#6830), [#7492](#7492), [#7563](#7563), and [#7582](#7582) -> `docs/changelog/2026-07-25.mdx`: Document the authoritative OpenClaw production lock, fixed managed-image dependencies, immutable Hermes base adoption, and Hermes image-size reduction. - [#7505](#7505), [#7530](#7530), [#7547](#7547), [#7508](#7508), [#7548](#7548), [#7549](#7549), [#7537](#7537), [#7534](#7534), [#7515](#7515), [#7511](#7511), [#7551](#7551), [#7562](#7562), [#7575](#7575), [#7496](#7496), [#7594](#7594), [#7595](#7595), and [#7599](#7599) -> `docs/changelog/2026-07-25.mdx`: Summarize release validation, transient and bounded dispatch reconciliation, exact pre-tag qualification, identity revalidation, npm-audit retry, sharding, image reuse, timeout, telemetry, and workflow-hardening changes. - Reconciled without separate changelog prose: - [#7539](#7539), [#7526](#7526), [#7507](#7507), [#7506](#7506), [#7519](#7519), [#7516](#7516), [#7396](#7396), [#7254](#7254), [#7583](#7583), [#7596](#7596), and [#7598](#7598): Test-harness or fixture-only changes. - [#7403](#7403), [#7161](#7161), [#6877](#6877), [#7531](#7531), [#7525](#7525), [#7522](#7522), [#7536](#7536), [#7552](#7552), [#7566](#7566), [#7553](#7553), [#7561](#7561), [#7577](#7577), [#7569](#7569), [#7585](#7585), [#7584](#7584), [#7592](#7592), [#7580](#7580), [#7571](#7571), [#7517](#7517), [#7589](#7589), [#7402](#7402), [#7558](#7558), [#7544](#7544), and [#7601](#7601): Dependency, internal recovery, validation, contributor-workflow, E2E optimization, telemetry, or CI trust changes with no separate user-facing release claim. - [#7556](#7556), [#7573](#7573), [#7576](#7576), and [#7578](#7578): Experimental repository-maintainer conflict automation with no canonical user documentation surface. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [x] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [ ] Tests added or updated for changed behavior - [x] Existing tests cover changed behavior — justification: `test/changelog-docs.test.ts` validates dated changelog structure, version headings, and published links. - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [ ] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: Reviewed `docs/changelog/2026-07-25.mdx` at exact head `0f5dedb47` against 90 first-parent release commits and 92 merged PRs labeled `v0.0.96`. Verified parser-safe MDX SPDX, the exact version heading, literal CLI names, writing style, skip terms, all 20 root-absolute published links, and the accepted #7591 opt-in authenticated discovery bounds. #7544, #7599, and #7601 remain internal or CI-only release-ledger entries. Changelog tests passed 6/6, the docs build passed with 0 errors and two pre-existing Fern warnings, and `npm run check:diff` plus the final diff check passed. - Agent: Codex Desktop documentation-writer subagent <!-- docs-review-head-sha: 0f5dedb --> <!-- docs-review-agents-blob-sha: be20a09 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run test/changelog-docs.test.ts`: 6/6 passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Not applicable to this prose-only changelog entry. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) — the build passed with 0 errors and 2 existing Fern warnings; the published-route check passed. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) — native changelog files use the required parser-safe MDX SPDX comment and no frontmatter. --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Persistent network policy exclusions with consistent restore/exclusion reporting across rebuilds/snapshots. * Opt-in MCP tool discovery via `mcp status --tools` with bounded, redacted authenticated traffic. * Improved HTTPS inference switching for custom endpoints and refreshed onboarding/model menu details. * Refined OpenShell gateway defaults for port `8080`, including more reliable readiness checks. * **Bug Fixes** * Prevent incorrect provider/model restoration after compatible-provider update failures. * Preserve managed MCP state after exec loss and tighten gateway/doctor status scoping. * **Tests** * Stronger, fail-closed release validation with hardened evidence/artifact handoff and bounded timeouts/retries. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com>
> **Supersedes #7802.** That branch could not be rewritten in place (the `No force push` > ruleset covers every ref except `main`), so this is a fresh branch carrying the same three > commits with GitHub-Verified history, rebased onto current `main`. > > @prekshivyas — your Bash 3.2 fix is cherry-picked here with your authorship intact > (`fix(sandbox): support Bash 3.2 label validation`), now signed so it clears the gate > alongside the other two. The focused command from your re-review still passes on this > head: `npx vitest run --project cli --project integration src/lib/onboard/sandbox-create-launch.test.ts test/repro-7795-connect-shell-sandbox-label.test.ts` -> 39/39. > No other content changed; `docs-review-receipt` re-runs automatically on the new head. <!-- markdownlint-disable MD041 --> ## Summary Inside a sandbox, every hint that prints a copyable host-side `nemoclaw <name> …` command rendered the literal `<name>` placeholder instead of the sandbox name, so the command could not be copied or scripted. Inject the host's validated sandbox name into every sandbox and resolve the hints from it. Closes #7795. ## Reproduction Executed on our DGX Spark aarch64 test host (GB10 GPU), against a sandbox freshly onboarded from `main` at `eeab81cc5`: 1. `node bin/nemoclaw.js onboard --name repro-7795 --non-interactive --yes` 2. `node bin/nemoclaw.js repro-7795 connect` (driven through a real PTY, not `exec`) 3. Inside the connect shell: `openclaw channels add discord` **Environment** - Test machine: our DGX Spark aarch64 test host (GB10 GPU), Ubuntu 24.04.4 LTS - Node.js v22.23.1, Docker 29.6.1, OpenShell CLI 0.0.85 - NemoClaw `main` @ `eeab81cc5542902538c97db63c132c0fdbd4341c` (`v0.0.96-45-geeab81cc5`) - Sandbox `repro-7795`, agent `openclaw`, provider `ollama-local` **Observed on `main` (before fix)** ```text See which rule denied a request: nemoclaw <name> logs --tail 50 MARKER_ENV=[1] Error: 'openclaw channels add' cannot modify channels inside the sandbox. Changes inside the sandbox do not persist across rebuilds. Run 'nemoclaw <name> channels add discord' on the host. ``` `MARKER_ENV` is `echo $OPENSHELL_SANDBOX` in the connect shell. **Observed on `fix/…` (after fix)** ```text NEMOCLAW_SANDBOX_NAME=repro-7795 # entrypoint process environment OPENSHELL_SANDBOX=1 export _NEMOCLAW_SANDBOX_LABEL='repro-7795' # baked into the connect-shell env See which rule denied a request: nemoclaw repro-7795 logs --tail 50 MARKER_ENV=[1] MARKER_LABEL=[repro-7795] Run 'nemoclaw repro-7795 channels add discord' on the host. Run 'nemoclaw repro-7795 channels remove slack' on the host. Run 'nemoclaw repro-7795 channels add <channel>' on the host. ``` The last line is `openclaw channels add "$(cat /etc/shadow)"`: the channel token still degrades to `<channel>` and no file content reaches the command, so the existing token allowlists are unaffected. ## Analysis `_nemoclaw_policy_denial_hint_label()` in `scripts/nemoclaw-start.sh` resolved the name from `OPENSHELL_SANDBOX`, documented there as carrying the sandbox name on OpenShell >= 0.0.44. That assumption does not hold for any process inside the sandbox. OpenShell records `OPENSHELL_SANDBOX=<name>` on the container, but the sandbox supervisor (PID 1) spawns sandbox processes with a rebuilt environment in which the variable is the boolean `1`. Measured on the test host: - container config / PID 1 environment: `OPENSHELL_SANDBOX=repro-7795` - `nemoclaw-start` entrypoint (runs as the unprivileged `sandbox` user): `OPENSHELL_SANDBOX=1`, and none of the other `OPENSHELL_*` values are present - interactive `connect` shell: `OPENSHELL_SANDBOX=1` The real value survives only in PID 1's environment, which is root-owned; the entrypoint runs as `sandbox` and gets `EACCES` on `/proc/1/environ`. The container hostname is the container ID, and no other in-container source carries the name. So the name was genuinely unavailable in-sandbox, and the allowlist correctly rejected `1`, falling back to the placeholder at every call site. This affected both consumers of the helper — the `openclaw channels add/remove` guard hint (`scripts/nemoclaw-start.sh:3774`, added in #7295) and the policy-denial logs breadcrumb (`scripts/nemoclaw-start.sh:3912`, added in #5978). Their unit tests pass only because they set `OPENSHELL_SANDBOX` to a name directly, which never happens in a real connect shell. The troubleshooting docs attributed real-name rendering to OpenShell 0.0.44 or newer. The reproduction on OpenShell 0.0.85 disproved that version distinction, so this PR updates both the implementation and the troubleshooting text. ## Fix `buildSandboxRuntimeEnvArgs()` already injected `NEMOCLAW_SANDBOX_NAME` into the sandbox startup command, but only for LangChain Deep Agents Code. Hoist that injection so every sandbox receives it. The value is the host's own `sandboxName`, already validated by `NAME_VALID_PATTERN` before a sandbox is created, and `NEMOCLAW_SANDBOX_NAME` is an existing documented NemoClaw variable with exactly this meaning — no new contract is introduced. `write_runtime_shell_env()` then bakes that name into the generated connect-shell env as `_NEMOCLAW_SANDBOX_LABEL`, and the renderer falls back to it when `OPENSHELL_SANDBOX` is unusable. Security properties: - The name is allowlisted at the bake site and again at the render site, against the same RFC-1123 pattern as before (`/^[a-z]([a-z0-9-]*[a-z0-9])?$/`, max 63), evaluated under `LC_ALL=C` in a subshell. Re-checking at render time matters because the sandbox can reassign the variable after the file is sourced. - The generator always emits either `export _NEMOCLAW_SANDBOX_LABEL='<name>'` or `unset _NEMOCLAW_SANDBOX_LABEL`, never nothing, so a value pre-set by the sandbox cannot survive into a copyable command when no valid name is available. - `OPENSHELL_SANDBOX` keeps priority when it carries a usable name, so a caller-provided valid runtime name overrides the generated fallback. - When no source yields a valid name the output is the previous `<name>` placeholder, so the failure mode is unchanged. Scope note: the remaining literal `nemoclaw <sandbox> …` strings in this file (the `channels login` guidance, the rebuild / `channels status` / `shields down` messages) are generic instructional text that does not echo a specific user invocation, matching the repo-wide documentation convention. #7295 deliberately replaced only the add/remove branch, so they are left as-is. Tests added: - `test/repro-7795-connect-shell-sandbox-label.test.ts` runs the real `write_runtime_shell_env` generator under the environment the entrypoint actually receives, then sources its output in a shell with `OPENSHELL_SANDBOX=1` — the connect-shell condition — and asserts the rendered hints. It covers both consumers, runtime-name precedence, boolean/empty/absent inputs, seven invalid inputs (including shell metacharacters, an ANSI escape with a newline, and command substitution), the 63-character limit, a sandbox-set label, a pre-set label that must be unset, and agreement with `NAME_VALID_PATTERN`. - `src/lib/onboard/sandbox-create-launch.test.ts` pins that every agent receives `NEMOCLAW_SANDBOX_NAME`, and that it is omitted when no name is known. Both would have failed before this change: the generator emitted no label, so the connect-shell assertions rendered `<name>`. ## Changes - `src/lib/onboard/sandbox-create-launch.ts`: inject `NEMOCLAW_SANDBOX_NAME` for every agent instead of LangChain Deep Agents Code only. - `scripts/nemoclaw-start.sh`: bake the validated name into the connect-shell env, and resolve the hint label from it when `OPENSHELL_SANDBOX` is unusable. - `src/lib/onboard/sandbox-create-launch.test.ts`: coverage for the injection. - `test/repro-7795-connect-shell-sandbox-label.test.ts`: end-to-end connect-shell regression coverage. - `docs/reference/troubleshooting.mdx`: describe the NemoClaw fallback without the incorrect OpenShell version distinction. - Review follow-up: keep the generator compatible with Bash 3.2 and state the fallback removal condition. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — the [maintainer review](#7802 (review)) found the security design sound. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: Updated `docs/reference/troubleshooting.mdx` to describe the NemoClaw-provided name without the incorrect OpenShell version distinction. - Agent: Codex Desktop <!-- docs-review-head-sha: 04b962a --> <!-- docs-review-agents-blob-sha: be20a09 --> ## Verification - [ ] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — `npx vitest run --project cli --project integration src/lib/onboard/sandbox-create-launch.test.ts test/repro-7795-connect-shell-sandbox-label.test.ts` passes 39 tests on Bash 3.2.57. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) — the docs build passes with 0 errors and 2 existing Fern warnings. - [x] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) Verified end to end on aarch64. The reporter notes the issue is not believed platform-specific, and the mechanism is connect-shell environment behavior rather than architecture, but an x86_64 confirmation before merge would close that gap. ## AI Disclosure - [x] AI-assisted — tools: Claude Code and Codex Desktop Signed-off-by: Yanyun Liao <yanyunl@nvidia.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved sandbox naming in policy-denial reminders and connect-shell guidance. - Displays the validated sandbox name when available, with a safe `<name>` fallback otherwise. - Ensures sandbox names are consistently propagated across supported agents. - Prevents invalid or unsafe sandbox names from appearing in generated command hints. - Runtime sandbox names now take priority when available. - **Documentation** - Updated troubleshooting guidance to explain sandbox-name display and the `nemoclaw list` fallback. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com> --------- Signed-off-by: Yanyun Liao <yanyunl@nvidia.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com> Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com>
Summary
The in-sandbox guard for
openclaw channels add/removeprinted a generic hint with literal<sandbox>and<channel>placeholders. Users and CI automation could not determine the correct host-side command from these generic tokens.Changes
_nemoclaw_policy_denial_hint_label()helper.addorremoveoperation; other values become<operation>.discord,slack,teams,telegram,wechat, orwhatsapp); missing or hostile values become<channel>.Before
After
Testing
npm run check:diffpassedCloses #7292
Type of Change
Quality Gates
Documentation Writer Review
no-docs-neededdocs/manage-sandboxes/messaging-channels.mdxalready says to use host-side channel commands and explains that in-sandbox channel mutations do not persist;docs/manage-sandboxes/add-channels-after-onboarding.mdx,docs/manage-sandboxes/manage-messaging-channels.mdx, anddocs/reference/commands.mdxalready show the exactchannels addandchannels removehost syntax. No command, flag, configuration, or procedure changed, and the docs do not quote the old error text. The current-main topper leaves the effective two-file product patch byte-identical to the independently reviewed patch (SHA-2563c77f46a2e7d2f2b78e6e9ba45875c34339ce01758c1cd29ff430eadce66f3a9).Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubnpm run check:diffalso passed after local ignored build artifacts were generatedbash -n scripts/nemoclaw-start.shSigned-off-by: kagura-agent kagura.agent.ai@gmail.com
Signed-off-by: Prekshi Vyas prekshiv@nvidia.com
Summary by CodeRabbit